Disentangle disparate 'bounds' ideas in processor #496

natecook1000 · 2022-06-17T19:55:18Z

This starts the separation of two different ideas for boundaries in the base input:

subjectBounds: These represent the actual subject in the input string. For a String callee, this will cover the entire bounds, while for a Substring these will represent the bounds of the substring in the base.
searchBounds: These represent the current search range in the subject. These bounds can be the same as subjectBounds or a subrange when searching for subsequent matches or replacing only in a subrange of a string.

This starts the separation of two different ideas for boundaries in the base input: - subjectBounds: These represent the actual subject in the input string. For a `String` callee, this will cover the entire bounds, while for a `Substring` these will represent the bounds of the substring in the base. - searchBounds: These represent the current search range in the subject. These bounds can be the same as `subjectBounds` or a subrange when searching for subsequent matches or replacing only in a subrange of a string.

We should be testing the higher-level Regex.firstMatch function instead of the lower-level types directly.

str.replacing(_:with:subrange:) still needs to use separate bounds for the subject and the search range.

When we move forward while searching for the first match, the search bounds should stay the same. Only the currentPosition needs to move forward. This will allow us to implement the \G start of match anchor, with which /\Gab/ matches "abab" twice, compared with /^ab/, which only matches once.

With this change, RegexMatchesCollection keeps the subject bounds and search bounds separately, modifying the search bounds with each iteration. In addition, the replace methods that only operate on a subrange can specify that specifically, getting the correct anchor behavior while only matching within a portion of a string.

natecook1000 · 2022-06-20T20:09:22Z

@swift-ci Please test

natecook1000 · 2022-06-20T18:55:36Z

Tests/RegexTests/MatchTests.swift

@@ -20,44 +20,17 @@ struct MatchError: Error {
    }
 }

-extension Executor {
-  func _firstMatch(


This test method was unused.

natecook1000 · 2022-06-20T18:56:56Z

Tests/RegexTests/MatchTests.swift

    firstMatchTest(#"["a-c"]+"#, input: "abc", match: "a",
-                   syntax: .experimental)
+                   syntax: .experimental, xfail: true)
    firstMatchTest(#"["abc"]+"#, input: "cba", match: "cba",
                   syntax: .experimental)
    firstMatchTest(#"["abc"]+"#, input: #""abc""#, match: "abc",
-                   syntax: .experimental)
+                   syntax: .experimental, xfail: true)


I don't know what the right behavior is for these experimental ones…

It's the same as the above, but we can also remove them. The quotes are just prettier \Q...\E

natecook1000 · 2022-06-20T18:57:25Z

Tests/RegexTests/MatchTests.swift

-  var executor = try _compileRegex(regex, syntax)
-  executor.engine.enableTracing = enableTracing
-  let (str, caps) = try executor._firstMatch(
-    regex, input: input, enableTracing: enableTracing)
-  let capStrs = caps.map { $0 == nil ? nil : String($0!) }
-  return (String(str), capStrs)
+  let regex = try Regex(regexStr)
+  guard let result = try regex.firstMatch(in: input) else {
+    throw MatchError("match not found for \(regexStr) in \(input)")
+  }
+  let caps = result.output.slices(from: input)
+  return (String(input[result.range]), caps.map { $0.map(String.init) })


Switched this to testing the actual public API instead of the executor.

Sources/_StringProcessing/Algorithms/Algorithms/Contains.swift

Sources/_StringProcessing/Algorithms/Matching/Matches.swift

Sources/_StringProcessing/ByteCodeGen.swift

Sources/_StringProcessing/Engine/Processor.swift

milseman · 2022-06-21T01:11:35Z

Sources/_StringProcessing/Engine/Processor.swift

@@ -411,9 +426,9 @@ extension Processor {

    case .consumeBy:
      let reg = payload.consumer
-      guard currentPosition < bounds.upperBound,
+      guard currentPosition < subjectBounds.upperBound,


Subject or search bounds?

How do we, generally, know which one to choose?

Generally, we want to be moving and matching with searchBounds; subjectBounds should really only be used to match anchors, not for general purpose index movements.

(i.e. this was incorrect)

Any way we could use access control or some better coding convention here? E.g. if we always want search bounds except for anchors, can we isolate the code that refers to subject bounds inside the engine?

Sources/_StringProcessing/Engine/Processor.swift

milseman · 2022-06-21T01:16:16Z

Tests/RegexTests/MatchTests.swift

    firstMatchTest(#"["a-c"]+"#, input: "abc", match: "a",
-                   syntax: .experimental)
+                   syntax: .experimental, xfail: true)
    firstMatchTest(#"["abc"]+"#, input: "cba", match: "cba",
                   syntax: .experimental)
    firstMatchTest(#"["abc"]+"#, input: #""abc""#, match: "abc",
-                   syntax: .experimental)
+                   syntax: .experimental, xfail: true)


It's the same as the above, but we can also remove them. The quotes are just prettier \Q...\E

This still isn't quite right - Processor.consume() should really return the matched range, since it already tracks the starting point of matching. Having it return the whole range will also prepare us for supporting `\K`, which will mean `currentPosition` isn't always the start of the matched range.

This at least clarifies which boundaries are being used.

natecook1000 · 2022-06-22T03:50:49Z

@swift-ci Please test

milseman

LGTM, some minor cleanups and testing requests.

Sources/_StringProcessing/Algorithms/Algorithms/Contains.swift

Sources/_StringProcessing/Algorithms/Matching/Matches.swift

Sources/_StringProcessing/ByteCodeGen.swift

Sources/_StringProcessing/Engine/Processor.swift

These anchors should always match at start/end of line, not just when .anchorsMatchLineEndings() is turned on.

natecook1000 · 2022-06-22T20:49:37Z

@swift-ci Please test

This separates the two different ideas for boundaries in the base input: - subjectBounds: These represent the actual subject in the input string. For a `String` callee, this will cover the entire bounds, while for a `Substring` these will represent the bounds of the substring in the base. - searchBounds: These represent the current search range in the subject. These bounds can be the same as `subjectBounds` or a subrange when searching for subsequent matches or replacing only in a subrange of a string. * firstMatch shouldn't update searchBounds on iteration When we move forward while searching for the first match, the search bounds should stay the same. Only the currentPosition needs to move forward. This will allow us to implement the \G start of match anchor, with which /\Gab/ matches "abab" twice, compared with /^ab/, which only matches once. * Make matches(of:) and ranges(of:) boundary-aware With this change, RegexMatchesCollection keeps the subject bounds and search bounds separately, modifying the search bounds with each iteration. In addition, the replace methods that only operate on a subrange can specify that specifically, getting the correct anchor behavior while only matching within a portion of a string.

natecook1000 added 8 commits June 17, 2022 14:53

Stop testing through the Executor

8b585af

We should be testing the higher-level Regex.firstMatch function instead of the lower-level types directly.

Fix firstMatch implementation

a6ff9a5

Stop using RegexConsumer in contains(_:)

a87bc1b

Update test xfailures

d1290f8

str.replacing(_:with:subrange:) still needs to use separate bounds for the subject and the search range.

Merge branch 'main' into substring-bounds

69dae18

natecook1000 changed the title ~~[WIP] Disentangle disparate 'bounds' ideas in processor~~ Disentangle disparate 'bounds' ideas in processor Jun 20, 2022

natecook1000 marked this pull request as ready for review June 20, 2022 22:20

natecook1000 requested a review from milseman June 20, 2022 22:20

natecook1000 commented Jun 20, 2022

View reviewed changes

milseman reviewed Jun 21, 2022

View reviewed changes

natecook1000 added 9 commits June 21, 2022 11:03

Improve subject/searchBounds documentation

46173f2

Processor.reset only needs the starting position

6dcb9fd

Audit search/subjectBounds usage

e80fe54

Further subject/searchBounds auditing

7c125bc

Update buildAssert use-sites to use subjectBounds

7bf5b14

This at least clarifies which boundaries are being used.

Additonal subject/searchBounds correctness.

6ce6108

Add internal API for testing alternative syntaxes

73ba7c7

Merge branch 'main' into substring-bounds

0da43e5

milseman approved these changes Jun 22, 2022

View reviewed changes

natecook1000 added 3 commits June 22, 2022 15:44

Add an XFAIL'd test for \G

c1a6edf

Add XFAIL'd tests for Anchor.start/endOfLine

8d8571f

These anchors should always match at start/end of line, not just when .anchorsMatchLineEndings() is turned on.

Improve FIXME comments

94612b2

natecook1000 merged commit e3e186f into swiftlang:main Jun 22, 2022

natecook1000 deleted the substring-bounds branch June 22, 2022 21:47

Disentangle disparate 'bounds' ideas in processor #496

Disentangle disparate 'bounds' ideas in processor #496

Uh oh!

Conversation

natecook1000 commented Jun 17, 2022

Uh oh!

natecook1000 commented Jun 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natecook1000 commented Jun 22, 2022

Uh oh!

milseman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

natecook1000 commented Jun 22, 2022

Uh oh!

Uh oh!